Skip to content

Conversation

@genedan
Copy link
Collaborator

@genedan genedan commented Jan 25, 2026

Closes #665.

@genedan
Copy link
Collaborator Author

genedan commented Jan 25, 2026

@jbogaardt, I may need some assistance on one of the failing notebooks, some changes last year caused this cell to fail, but it ventures into some code that I have yet to figure out. The reprex is:

import chainladder as cl
s1 = cl.load_sample('clrd').iloc[:3]
s2 = s1.sort_index(ascending=False)
s3 = s1.iloc[:, ::-1]

s1 + s3
image

@codecov
Copy link

codecov bot commented Jan 25, 2026

Codecov Report

❌ Patch coverage is 92.85714% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 85.00%. Comparing base (ad4d081) to head (7e1dd27).
⚠️ Report is 1 commits behind head on master.

Files with missing lines Patch % Lines
chainladder/core/pandas.py 92.30% 2 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master     #666      +/-   ##
==========================================
- Coverage   85.10%   85.00%   -0.11%     
==========================================
  Files          85       85              
  Lines        4869     4883      +14     
  Branches      619      626       +7     
==========================================
+ Hits         4144     4151       +7     
- Misses        517      522       +5     
- Partials      208      210       +2     
Flag Coverage Δ
unittests 85.00% <92.85%> (-0.11%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@genedan
Copy link
Collaborator Author

genedan commented Jan 25, 2026

The checks have passed. There's a bunch going on here, so to sum it up:

  • Primary goal of the PR is to include the docs notebooks in the unit tests so that we no longer have to manually read the docs to catch errors. Closes Apply pytest to documentation notebooks #665.
  • This involved installing a dependency, nbmake
  • Upon testing the notebooks, a number of issues were found and resolved:
    • Notebooks had dependencies that were not listed in pyproject.toml. These would be jupyter-black and statsmodels. This closes Fix lab_black usage in docs #608.
    • There was a cell in one of the tutorials that referenced an incorrect name for an axis, "segments", causing a bug. I replaced this with "columns".
    • Another bug was discovered involving the summation of triangles with reordered columns, which was resolved, closes Triangle addition fails for triangles with the same columns, but different ordering. #667.
    • The JSON of the blank sandbox workbook was edited to exclude it from unit testing since test failure was caused by placeholder code, which should be OK since it's a fill-in-the-blank style notebook.
    • TrianglePandas.to_frame was annotated, it was not meant to be in the initial scope of the PR, but was committed beforehand, so it's included here.
    • Codecov lists 3 lines in TrianglePandas.to_frame that aren't covered. They weren't covered beforehand, so it doesn't impact the total coverage from before the PR. I think this should be addressed in a later PR to keep the scope from getting bigger.

Lastly, there are some previous commits that I thought were merged in previous PRs, I'd request from the reviewer to please let me merge this PR myself after approving so I can get them cleared.

@henrydingliu
Copy link
Collaborator

henrydingliu commented Jan 26, 2026

@genedan since this PR contains a decent amount doc changes, can you please add it to a version on RTD so it's easier to review?

@kennethshsu do we want to add black as a dependency?

"ename": "ValueError",
"evalue": "Invalid axis specified. Please specify the correct string or integer representation of the desired axis.",
"ename": "NameError",
"evalue": "name 'clrd' is not defined",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

appears to be an error

@genedan
Copy link
Collaborator Author

genedan commented Jan 26, 2026

@henrydingliu, how would I go about adding a version to RTD? This is my first time updating the docs, so I'm not sure what the procedure is (contributing to the RTD account directly vs. hosting a docs of my own fork, etc.).

@henrydingliu
Copy link
Collaborator

on your own fork, you can create an RTD account, connect it to your github account, and then follow the RTD GUI to set up a new RTD instance for your fork. I did this for myself. I think this is fine for tinkering. But I think it's a little less collaborative. Which is where I'd like to explore creating versions of the doc on the main repo. But I haven't figured it out. you can see my failed attempts.

@jbogaardt why didn't creating a branch called v0.8.x automatically start a new build on stable?

@kennethshsu
Copy link
Collaborator

@kennethshsu do we want to add black as a dependency?

Sorry that I'm a bit late to the discussion, I don't really know... I feel like this is forcing the users to use black, and I know some prefer another linter...

@genedan
Copy link
Collaborator Author

genedan commented Jan 27, 2026

I've created a RTD of my fork: https://gd-cl-dev.readthedocs.io

Regarding jupyter-black, I'll defer to you guys. It's not part of the core API and relatively easy to add/remove from the docs, so adding it to the docs/test sections of pyproject.toml as a dependency isn't a big deal to me, and with this PR, we'll be able to catch any problems with it in the future.

@kennethshsu
Copy link
Collaborator

That's great - then my vote is to remove it because it's not a true dependency.

@genedan
Copy link
Collaborator Author

genedan commented Jan 27, 2026

@kennethshsu, would you also be okay with removing it from the cells where it appears in the docs? From a search, there are ~8 instances where it appears in the docs: https://chainladder-python.readthedocs.io/en/latest/search.html?q=jupyter_black#.

If you could confirm, I'll get started with removing it from the notebooks.

@kennethshsu
Copy link
Collaborator

Yes I think that's fine, or just comment it out and write that it's only a linter, which is what I used to do.

I probably prefer that they are commented out, so people know that the notebooks are linted, and the linter used is black.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Apply pytest to documentation notebooks

3 participants